Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Select windows by index #1152

Merged
merged 1 commit into from
Oct 4, 2023
Merged

Select windows by index #1152

merged 1 commit into from
Oct 4, 2023

Conversation

adrienmaillard
Copy link
Contributor

@adrienmaillard adrienmaillard commented Sep 25, 2023 •
edited
Loading

  • Tickets addressed: AERIE-000
  • Review: By commit
  • Merge strategy: Merge (no squash)

Description

This allows to select windows by their index in the sequence.
Example: Real.Resource("state of charge").lessThan(0.3).starts().keepTrueSegment(2) will select the second window of Real.Resource("state of charge").lessThan(0.3).starts(). If it does not exist, it will return nothing.

If the user provides a negative index, the index starts from the last element (-1).

Verification

Some new tests.

Documentation

Has to be added to constraints documentation.

Future work

I want to add before(Windows) and after(Windows) operators to support more sequence operations.

@adrienmaillard adrienmaillard requested a review from a team as a code owner September 25, 2023 22:02
@adrienmaillard adrienmaillard temporarily deployed to e2e-test September 25, 2023 22:02 — with GitHub Actions Inactive
@adrienmaillard adrienmaillard removed the request for review from mattdailis September 25, 2023 22:02
@adrienmaillard adrienmaillard temporarily deployed to e2e-test September 25, 2023 23:28 — with GitHub Actions Inactive
@JoelCourtney
Copy link
Contributor

Cool! The java Windows class already has a keepTrueSegment function which should have the same behavior. The AST node should only have to delegate to that function.

I'd prefer if it was named keepTrueSegment or something similarly descriptive, because to me index suggests that it selects the n'th segment, not the n'th true segment. I know most users don't think that way because much of the API prioritizes true segments, but we are going to have to be moving the mental model in that direction for procedural constraints anyway.

@adrienmaillard
Copy link
Contributor Author

adrienmaillard commented Sep 26, 2023 •
edited
Loading

Cool! The java Windows class already has a keepTrueSegment function which should have the same behavior. The AST node should only have to delegate to that function.

I'd prefer if it was named keepTrueSegment or something similarly descriptive, because to me index suggests that it selects the n'th segment, not the n'th true segment. I know most users don't think that way because much of the API prioritizes true segments, but we are going to have to be moving the mental model in that direction for procedural constraints anyway.

Oh you're right, I was not aware. I'll replace the ad-hoc code with a call to keepTrueSegment. For the name, the closest I can agree to is getTrueSegment. keepTrueSegment works for getting a set of intervals but its meaning is odd if it is supposed to return only one window. Other ideas ?

@adrienmaillard adrienmaillard temporarily deployed to e2e-test October 2, 2023 21:25 — with GitHub Actions Inactive
@adrienmaillard adrienmaillard temporarily deployed to e2e-test October 2, 2023 21:31 — with GitHub Actions Inactive
@adrienmaillard
Copy link
Contributor Author

Actually, I like keepTrueSegment as it reflects better what's done. I have made the changes. If you want to review, it's ready ;)

@adrienmaillard adrienmaillard temporarily deployed to e2e-test October 2, 2023 23:56 — with GitHub Actions Inactive
@adrienmaillard adrienmaillard temporarily deployed to e2e-test October 4, 2023 17:03 — with GitHub Actions Inactive
@adrienmaillard adrienmaillard temporarily deployed to e2e-test October 4, 2023 17:14 — with GitHub Actions Inactive
@adrienmaillard adrienmaillard merged commit a491c5a into develop Oct 4, 2023
6 checks passed
@adrienmaillard adrienmaillard deleted the add-index-constraint branch October 4, 2023 17:28
@mattdailis mattdailis added constraints Anything related to the constraints domain feature A new feature or feature request labels Oct 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JoelCourtney JoelCourtney JoelCourtney approved these changes

Assignees

@adrienmaillard adrienmaillard

Labels
constraints Anything related to the constraints domain feature A new feature or feature request
Projects
Aerie
Archived in project
Milestone
FY23 Q4 - Ad Hoc Improvements
Development

Successfully merging this pull request may close these issues.
3 participants
@adrienmaillard @JoelCourtney @mattdailis